Move Transfer.storage_locations_transferred_to/from_in to StorageLoca… #4733
Conversation
app/models/storage_location.rb
Outdated
| @@ -77,6 +77,14 @@ def self.items_inventoried(organization, inventory = nil) | |||
| end | |||
| end | |||
|
|
|||
| def self.with_transfers_to(organization) | |||
| joins(:transfers_to).where(organization_id: organization.id).distinct.order(:name) | |||
There was a problem hiding this comment.
You shouldn't need the where clause since the transfer has a from_id and to_id which powers the association.
There was a problem hiding this comment.
Without the where clause, wouldn't we be grabbing every storage location that has at least one transfer from/to that location? Don't we only want the storage locations related to a certain organization?
There was a problem hiding this comment.
Storage locations belong to an organization and so do transfers. So if you have a transfer, by definition only storage locations belonging to the same organization can be involved in it.
There was a problem hiding this comment.
Sorry I still don't understand why this where clause is unnecessary. We are only joining the transfers and storage_locations tables on the storage_locations primary key and the transfers foreign keys from_id and to_id. So it will return records for different organizations unless we specify an organization_id somewhere. (Or like if we moved the method to be an instance method on Organization so the id is specified by default.)
To illustrate in rails console in development:
-
Organization 1 has a transfer from storage location 1 to storage location 2. Organization 2 has a transfer from storage location 3 to storage location 4.
-
If I remove the
whereclause, then I get all the storage locations that have been transferred to (locations 2 and 4), which correspond to different organizations.
-
When I add back in the
whereclause, I see only storage locations that apply to a certain organization which I thought is what we wanted.
Am I missing something?
There was a problem hiding this comment.
Nope, I was missing the fact that this was a class-level method instead of an instance-level method.
I think this could be clearer if you made these scopes instead of class-level methods.
There was a problem hiding this comment.
Ok changed them to be scopes instead 👍
app/models/storage_location.rb
Outdated
| @@ -77,6 +77,14 @@ def self.items_inventoried(organization, inventory = nil) | |||
| end | |||
| end | |||
|
|
|||
| def self.with_transfers_to(organization) | |||
| joins(:transfers_to).where(organization_id: organization.id).distinct.order(:name) | |||
There was a problem hiding this comment.
Nope, I was missing the fact that this was a class-level method instead of an instance-level method.
I think this could be clearer if you made these scopes instead of class-level methods.
coalest
force-pushed
the
Refactor-transfer-scope-to-storage-location-model
branch
from
053f8c5
to
f2100cd
Compare
coalest
force-pushed
the
Refactor-transfer-scope-to-storage-location-model
branch
2 times, most recently
from
16469cf
to
9d20bbb
Compare
coalest
force-pushed
the
Refactor-transfer-scope-to-storage-location-model
branch
from
9d20bbb
to
1c57032
Compare
|
@dorner I rebased onto main and fixed the merge conflicts (keeping the same commit history). Should be good to go. |
|
Yeah, I do, but I've run out of time for today. |
|
LGTM functionally. |
(Doesn't resolve any issue)
Description
While working on something else, I noticed this
Transferclass method that seemed odd in that it returns an array ofStorageLocationActiveRecord objects. Feels like it belongs more on theStorageLocationmodel. Moving it also simplified the method logic.Also fixed the association's
foreign_keyso that ActiveRecord can join the table correctly.Type of change
Refactor
How Has This Been Tested?
Moved model spec from
TransfertoStorageLocation.